CI: enable errorlint #2512

kolyshkin · 2024-08-10T02:46:31Z

This improves error reporting and handling, whether it makes sense, enables errorlint, and silences some of its warnings.

See individual commits for details.

When commit 849dd70 was written, Go did not have an ability to wrap multiple errors. Since Go 1.20 it's possible by either using multiple %w, or via errors.Join (which discards nil errors). In a sense, this is what the code was doing before commit 849dd70, except we don't add extra spaces or parentheses -- as joined error, when converted to a string, is represented with newlines added between elements. Signed-off-by: Kir Kolyshkin <[email protected]>

1. Use os.ReadDir to remove a few lines of code. 2. Don't wrap errors from os, they already contain file name. Signed-off-by: Kir Kolyshkin <[email protected]>

1. Don't add context (such as file name and operation) to errors from "os" as they already have this information. 2. Use %w for errors, including multiple errors (works Go 1.20). Signed-off-by: Kir Kolyshkin <[email protected]>

This needs to be "require" rather than "assert", as if error is returned, test needs to fail now. Otherwise, close will panic. Signed-off-by: Kir Kolyshkin <[email protected]>

The current code is unnecessarily complicated: - check if a file/directory exists before opening it unnecessary; - it uses three helper functions not used anywhere else; - directory contents is read twice. Untangle all this, making the code simpler. Signed-off-by: Kir Kolyshkin <[email protected]>

Use errors.As to convert errors. Move the code that converts JSONFormatError to NewInvalidSignatureError to a separate helper, and use it everywhere. Remove the intermediary functions where they became unnecessary. Signed-off-by: Kir Kolyshkin <[email protected]>

This enables errorlint to golangci-lint configuration, and silences last warnings found (three of these are to be removed once a new version of errorlint will be used). Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2024-08-10T02:48:44Z

Let me know if you want me to separate the commit titled directory: simplify newImageDestination into its own PR.

mtrmac

Thanks!

I think there are valuable parts in here (e.g. starting to use %w in code written before it was available), but on the most frequent themes, this is IMHO just not an improvement.

The Go error “trees” combining multiple error causes make everything worse
We can’t stop reporting file paths because fs.PathError’s output is ambiguous
Every single warning from the linter was either silenced with a comment, or has resulted in an unwanted change to how errors are reported. Meanwhile, the linter does not report the legacy code which should have been using %w. In total, the linter is a 100% negative for the project AFAICS, and shouldn’t be enabled.

mtrmac · 2024-08-10T19:58:15Z

copy/copy.go

-			} else {
-				retErr = fmt.Errorf(" (dest: %v)", err)
-			}
+			retErr = errors.Join(retErr, fmt.Errorf("dest: %w", err))


I think using the standard-library errors.Join is basically never valuable.

If propagated all the way to the CLI, the formatting with embedded newlines doesn’t work well with the (admittedly naive, but extremely widespread) practice of just including an error string in a single-line error message (in the worst case, in the middle). And even if the CLI code wanted to nicely format the individual error values, that’s only possible if the value returned by errors.Join is not wrapped itself (because that wrap-around-Join probably reformats the an error message and the CLI layer has no idea whether it is correct to discard that updated error message formatting).

IOW, if nothing else, I think errors.Join should be replaced by carefully designing the resulting text. (Compare internal/multierr.Format).

For Go-native handling, I don’t know what errors.Is(errors.Join(…)) means. The only reasonable case I can think of is that there is a single underlying error cause, and a multi-error Is is used to match that cause against multiple error types, e.g. for compatibility with various APIs. If the error value actually reports multiple independent error situations / causes (as in here), I don’t know useful action the caller could ever take based on an errors.Is operation.

This is particularly bad if multiple conditions are combined: errors.Is(err, RemoteServerError) && !errors.Is(err, RemoteServerOverloadedErr) breaks if the two branches can match two different error causes.

And in particular, in these cleanup situations, the caller almost certainly doesn’t want to react to the cause of the Close failure if retErr has already been set. So I think type-erasing the err and only including the string is the more correct thing to do.

Done that in #2519.

mtrmac · 2024-08-10T20:03:28Z

docker/body_reader.go

@@ -190,7 +190,7 @@ func (br *bodyReader) Read(p []byte) (int, error) {
 			return n, fmt.Errorf("%w (after reconnecting, server did not process a Range: header, status %d)", originalErr, http.StatusOK)
 		default:
 			err := registryHTTPResponseToError(res)
-			return n, fmt.Errorf("%w (after reconnecting, fetching blob: %v)", originalErr, err)
+			return n, fmt.Errorf("%w (after reconnecting, fetching blob: %w)", originalErr, err)


See errors.Join elsewhere — I don’t see why it’s valuable for the callers’ errors.Is to match against two different causes here. Only reporting the primary failure cause seems to me far more consistent.

mtrmac · 2024-08-10T20:09:08Z

docker/distribution_error.go

@@ -99,7 +99,7 @@ func parseHTTPErrorResponse(statusCode int, r io.Reader) error {
 }

 func makeErrorList(err error) []error {
-	if errL, ok := err.(errcode.Errors); ok {
+	if errL, ok := err.(errcode.Errors); ok { //nolint:errorlint


If this should happen at all, please include the actual warning being silenced, throughout.

mtrmac · 2024-08-10T20:10:24Z

docker/docker_image_src.go

@@ -798,7 +798,7 @@ func (n *bufferedNetworkReader) read(p []byte) (int, error) {
 	select {
 	case b = <-n.readyBuffer:
 		if b.err != nil {
-			if b.err != io.EOF {
+			if b.err != io.EOF { //nolint:errorlint


Returning exactly io.EOF is a clearly documented API, so this just indicates an impractical linter to me.

errorlint has a list of exceptions for that, listing functions that can return literal io.EOF but the mechanism doesn't work for some of the more complex setups (including, apparently, this one, when an error is part of a struct which is passed through a channel).

mtrmac · 2024-08-10T20:22:35Z

docker/archive/writer.go

@@ -34,7 +34,7 @@ func NewWriter(sys *types.SystemContext, path string) (*Writer, error) {
 	// only in a different way. Either way, it’s up to the user to not have two writers to the same path.)
 	fh, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE, 0644)
 	if err != nil {
-		return nil, fmt.Errorf("opening file %q: %w", path, err)
+		return nil, err


Ugh, I just realized fs.PathError does not quote the path.

Can’t be helped, really — even if we wanted to wrap all os/io/… calls with something that tries to fix that up after-the-fact, it’s impossible to do reliably because fs.PathError could itself be wrapped somewhere inside the standard library.

@$#!@$#!@ Go and its worse-is-better designs @$#!@$#!@

So, ultimately, I think all these Errorf(%q: %w) should stay. They are redundant but at least they can unambiguously identify weird inputs and attacks. (Sadly, even if we instituted a project-wide policy to always do that, there’s little hope of getting 100% coverage including dependencies.)

I think that using %q (rather than %s or %v) when logging errors should solve the issue of non-printable characters in file names. The only missing piece would be quotes around the name but it is probably very obvious where we the file name begins and ends (even with extra spaces): https://go.dev/play/p/OmxkoY3DJwS

So I guess if we use %q for errors everywhere we log/print them (which is probably a doable task), we can omit wrapping errors from os for the sake of adding a file name.

Oh BTW this was not caused by warnings from errorlint.

So I guess if we use %q for errors everywhere we log/print them (which is probably a doable task), we can omit wrapping errors from os for the sake of adding a file name.

Except, of course, double quotes will not be needed in this case. Hmm...

Overall, I think, while it's nice to have non-printable characters quoted, it's not crucial to have. If we're worried about attacks with weird inputs, those inputs need to be validated.

Do you think opening a Go proposal to quote the file name in '(*PathError).Error` makes sense?

I think that using %q (rather than %s or %v) when logging errors should solve the issue of non-printable characters in file names.

I don’t think that’s ever going to happen at the top level, e.g. because printing an error which does use %q itself would look horrible.

it is probably very obvious where we the file name begins and ends (even with extra spaces):

Consider /home/known-user-name/looks/like/an/innocuous/typo: no such file or directory\nexpected failure, please ignore:. (Even worse, if we had errors.Join routinely generating error texts that do include \n.)

Yeah, wrapping and joining indeed makes things less comprehensible. The only real solution to that, I guess, is structured logging.

I also played with having an os package which wraps all the standard os functionality, adding path quoting in (*PathError).Error and (*LinkError).Error methods. It sorta works, but I don't like this approach, so I'm dropping the ball here.

signature/internal/rekor_set.go

mtrmac · 2024-08-10T20:27:33Z

storage/storage_dest.go

@@ -239,7 +239,7 @@ func (s *storageImageDestination) putBlobToPendingFile(stream io.Reader, blobinf
 	filename := s.computeNextBlobCacheFile()
 	file, err := os.OpenFile(filename, os.O_CREATE|os.O_TRUNC|os.O_WRONLY|os.O_EXCL, 0600)
 	if err != nil {
-		return private.UploadedBlob{}, fmt.Errorf("creating temporary file %q: %w", filename, err)
+		return private.UploadedBlob{}, err


I think it’s somewhat valuable to explain why the user is suddenly seeing a path what wasn’t in the input.

mtrmac

.

mtrmac · 2024-08-10T20:43:32Z

directory/directory_dest.go

-	if dirExists {
-		isEmpty, err := isDirEmpty(ref.resolvedPath)
-		if err != nil {
+	dir, err := os.OpenFile(ref.resolvedPath, syscall.O_DIRECTORY|syscall.O_NOFOLLOW|os.O_RDONLY, 0)


This code must remain cross-platform.

Strictly speaking, neither syscall flags are needed here.

O_DIRECTORY is not needed here since readdir below will fail anyway

O_NOFOLLOW is here to avoid security issues when a malicious symlink is placed in the directory. If we trust the directory, we can skip this.

Do you want me to drop these, or have these two flags used when on Unix only?

Reading a bit more carefully: in the first place, it’s not defined at all that syscall.O_ constants are valid in calls to os.OpenFile.

I don’t see what threat the O_NOFOLLOW is protecting against. [Opening a directory and using *at for all operations would be a thought, but that’s not what we are doing, and even then I don’t why O_NOFOLLOW would be necessary.]

So I’m fine with dropping both.

Done that in #2520

kolyshkin · 2024-08-15T07:13:36Z

OK I will now try to salvage the good parts from here.

kolyshkin · 2024-08-15T08:12:44Z

Done salvaging, this one is no longer needed.

kolyshkin added 7 commits August 9, 2024 19:44

pkg/blobcache: simplify test case

1d4025f

1. Use os.ReadDir to remove a few lines of code. 2. Don't wrap errors from os, they already contain file name. Signed-off-by: Kir Kolyshkin <[email protected]>

Improvements wrt errors

aa038a5

1. Don't add context (such as file name and operation) to errors from "os" as they already have this information. 2. Use %w for errors, including multiple errors (works Go 1.20). Signed-off-by: Kir Kolyshkin <[email protected]>

directory: test case nit

61852e6

This needs to be "require" rather than "assert", as if error is returned, test needs to fail now. Otherwise, close will panic. Signed-off-by: Kir Kolyshkin <[email protected]>

Enable errorlint, ignore some warnings

2f691ef

This enables errorlint to golangci-lint configuration, and silences last warnings found (three of these are to be removed once a new version of errorlint will be used). Signed-off-by: Kir Kolyshkin <[email protected]>

mtrmac reviewed Aug 10, 2024

View reviewed changes

mtrmac requested changes Aug 10, 2024

View reviewed changes

mtrmac reviewed Aug 10, 2024

View reviewed changes

This was referenced Aug 15, 2024

Introduce/use JSONFormatToInvalidSignatureError #2518

Merged

Use %w to wrap errors, enable errorlint #2519

Merged

directory: simplify newImageDestination #2520

Open

pkg/blobcache: simplify test case #2521

Merged

kolyshkin closed this Aug 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: enable errorlint #2512

CI: enable errorlint #2512

kolyshkin commented Aug 10, 2024

kolyshkin commented Aug 10, 2024

mtrmac left a comment

mtrmac Aug 10, 2024 •

edited

Loading

kolyshkin Aug 15, 2024

mtrmac Aug 10, 2024

mtrmac Aug 10, 2024

mtrmac Aug 10, 2024

kolyshkin Aug 11, 2024

mtrmac Aug 10, 2024

kolyshkin Aug 11, 2024

kolyshkin Aug 12, 2024

mtrmac Aug 12, 2024

kolyshkin Aug 15, 2024

mtrmac Aug 10, 2024

mtrmac left a comment

mtrmac Aug 10, 2024

kolyshkin Aug 11, 2024

mtrmac Aug 12, 2024

kolyshkin Aug 15, 2024

kolyshkin commented Aug 15, 2024

kolyshkin commented Aug 15, 2024

CI: enable errorlint #2512

CI: enable errorlint #2512

Conversation

kolyshkin commented Aug 10, 2024

kolyshkin commented Aug 10, 2024

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin commented Aug 15, 2024

kolyshkin commented Aug 15, 2024

mtrmac Aug 10, 2024 •

edited

Loading